Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

job-list: misc cleanup #5407

Merged
merged 5 commits into from
Aug 23, 2023
Merged

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Aug 23, 2023

just splicing out some random cleanup before the bigger job-list PR that's upcoming.

chu11 added 2 commits August 22, 2023 21:00
Problem: In job-list/test/job_data.c, the function read_file() returns
a file descriptor that no one uses.

Have read_file() return void instead of the file descriptor.  Simply close
the file descriptor before returning.
Problem: In the header file job_data.h, the comments for what fields
are parsed in specific parsing functions is out of date.

Update the headers appropriately for all fields parsed by those functions.
@chu11 chu11 force-pushed the job_list_misc_cleanup branch from 62c5633 to 9d48a7a Compare August 23, 2023 22:22
Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK To me!

@@ -266,13 +266,12 @@ static int parse_jobspec (struct job *job, const char *s, bool allow_nonfatal)
json_error_t error;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the commit message for "job-list: do not return error when nonfatal set" there's a duplicate jobspec word: "illegal json jobspec jobspec"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops! thanks. i'll fix up then set MWP.

chu11 added 3 commits August 23, 2023 15:55
Problem: The internal function parse_jobspec() returns an error for
an illegal json jobspec when the 'allow_nonfatal' flag is set.

Do not return on error when 'allow_nonfatal' is set to true.
Problem: There are several tests in t2260-job-list.t that test
that job-list works if R is illegal.  However, they only test
when R is an illegal formatted json object.  They do not test
if R is missing expected fields.

Add additional tests that set R to an empty json object.
Problem: In the internal state transition struct, the flag "processed"
is a little ambiguous.  In addition, multiple locations manually
remove a state transition from the next_states list.

Solution: Split the "processed" flag into two flags, "processing" and
"finished".  The "processing" flag more clearly indicates that the state
transition is being processed.  The "finished" flag indicates the transition
is done and we can remove the struct from the next_states list.  With these
new flags, we no longer need to remove the struct from the next_states list
in so many locations.  Simply set the "finished" flag to true and remove
from the list in process_next_state().
@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #5407 (cc20c39) into master (bda266f) will increase coverage by 0.00%.
The diff coverage is 88.88%.

@@           Coverage Diff           @@
##           master    #5407   +/-   ##
=======================================
  Coverage   83.54%   83.55%           
=======================================
  Files         470      470           
  Lines       78577    78582    +5     
=======================================
+ Hits        65646    65658   +12     
+ Misses      12931    12924    -7     
Files Changed Coverage Δ
src/modules/job-list/job_state.c 77.25% <87.50%> (+0.18%) ⬆️
src/modules/job-list/job_data.c 93.04% <100.00%> (-0.04%) ⬇️

... and 8 files with indirect coverage changes

@mergify mergify bot merged commit dc07110 into flux-framework:master Aug 23, 2023
30 checks passed
@chu11 chu11 deleted the job_list_misc_cleanup branch August 24, 2023 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants